src: remap invalid file descriptors using `dup2` #44461

obiwac · 2022-08-30T21:49:02Z

When checking for the validity of the stdio file descriptors in PlatformInit(), this was the code previously used (as in #875):

// Make sure file descriptors 0-2 are valid before we start logging anything.
for (int fd = STDIN_FILENO; fd <= STDERR_FILENO; fd += 1) {
  struct stat ignored;
  if (fstat(fd, &ignored) == 0)
    continue;
  // Anything but EBADF means something is seriously wrong.  We don't
  // have to special-case EINTR, fstat() is not interruptible.
  if (errno != EBADF)
    abort();
  if (fd != open("/dev/null", O_RDWR))
    abort();
}

As I understand it, the intention behind this is to map file descriptors to /dev/null if they don't exist, by hoping the next file descriptor to be given will be the one which doesn't exist. This is imperfect as it is very platform dependent and breaks when /dev/null has already been given a different file descriptor in the same process, but e.g. stdin has disappeared in the meantime (as is the case with this issue on FreeBSD with the daemon tool when not passing -f: nodejs/help#2411).

Instead, this patch uses the dup2(2) syscall to properly remap the missing stdio file descriptor to what /dev/null's file descriptor actually is.

nodejs-github-bot · 2022-08-30T21:49:07Z

Review requested:

@nodejs/startup

src/node.cc

addaleax · 2022-08-31T12:16:27Z

by hoping the next file descriptor to be given will be the one which doesn't exist

“hoping” is doing a lot of work here 🙂 This was actually spec’d behavior in previous POSIX versions, and I was quite surprised to learn (through this PR) that the spec actually changed in this regard.

I’d leave a comment in the source code about this, since others might be surprised by this as well.

obiwac · 2022-08-31T12:51:59Z

I’d leave a comment in the source code about this, since others might be surprised by this as well.

Got you. I'll do that in a little bit ✌️

nodejs-github-bot · 2022-08-31T20:36:29Z

CI: https://ci.nodejs.org/job/node-test-pull-request/46354/

bnoordhuis

TBH, this change adds more failure paths and I don't think it's actually necessary.

The "open() returns the lowest available file descriptor" convention is so deeply codified in the UNIX ecosystem that it's never going away, no matter what POSIX says. There's simply too much software built around that assumption.

src/node.cc

obiwac · 2022-09-01T11:04:06Z

The "open() returns the lowest available file descriptor" convention is so deeply codified in the UNIX ecosystem that it's never going away, no matter what POSIX says.

While I do agree with this, there still are specific cases where "available" means something different to what's expected. Case in point:

#include <fcntl.h>
#include <stdio.h>
#include <stdlib.h>

int main(void) {
	system("echo && fstat | grep a.out");
	int fd = open("/dev/null", O_RDWR);
	printf("\n/dev/null: %d\n", fd);

	return 0;
}

Running this on FreeBSD in a separate session with daemon -o log.log -r /tmp/a.out and then closing said session (thus making stdin invalid):

root     a.out      89214 text [restricted]    519 -rwxr-xr-x   15424  r
root     a.out      89214   wd /        107129 drwxr-xr-x      16  r
root     a.out      89214 root /        107106 drwxr-xr-x      24  r
root     a.out      89214 jail /        107106 drwxr-xr-x      24  r
root     a.out      89214    0 -         -         bad    -
root     a.out      89214    1* pipe fffff8002e0e7d00 <-> fffff8002e0e7ba0      0 rw
root     a.out      89214    2* pipe fffff8002e0e7d00 <-> fffff8002e0e7ba0      0 rw
root     a.out      89214    3 [restricted]     24 crw-rw-rw-    null rw

/dev/null: 3

(This is actually the issue nodejs/help#2411 had.)

Here, 0 should be available, yet /dev/null is still opened as 3. Something else that works is closing 0 before opening /dev/null, but IMHO at that point it's kinda relying on flaky behaviour.

When checking for the validity of the stdio file descriptors (nodejs#875), ones which don't exist are intended to be remapped to /dev/null (and, if that doesn't work, we abort). This however doesn't work on all platforms and in all cases, and is not anymore required by POSIX; instead, use the `dup2` syscall as a more robust solution (conforms to POSIX.1). Fixes: nodejs/help#2411 Refs: nodejs#875

bnoordhuis · 2022-09-02T09:58:16Z

Ah okay, fair enough. Fd 0 points to a kind of zombie file description in your example, i.e., it's technically still open but not actually usable?

obiwac · 2022-09-04T21:12:51Z

Fd 0 points to a kind of zombie file description in your example, i.e., it's technically still open but not actually usable?

AFAIU the file descriptor is never actually closed, so it isn't ever marked as unused (fdunused()), which is why it's never removed from the file descriptor table.

bnoordhuis · 2022-09-05T20:47:00Z

Wait, but fstat() still fails with EBADF? How does that work?

obiwac · 2022-09-06T21:37:11Z

I don't know, there are plenty of places along the line where fstat(2) can fail with EBADF, not just because a file descriptor is not found in the file descriptor table (you can check out kern_fstat(), my quick guess is that fp->f_ops is set to &badfileops at some point before being unused).

I could investigate further if you'd like an exact answer but I don't see how it's too relevant outside of the FreeBSD kernel :P

bnoordhuis · 2022-09-08T09:04:04Z

"Zombie file descriptor" is good enough for me. :-)

nodejs-github-bot · 2022-09-08T09:09:36Z

CI: https://ci.nodejs.org/job/node-test-pull-request/46479/

obiwac · 2022-10-25T12:58:13Z

Is there anything left for me to do here?

nodejs-github-bot · 2022-10-25T13:01:27Z

CI: https://ci.nodejs.org/job/node-test-pull-request/47466/

nodejs-github-bot · 2022-10-26T16:31:28Z

Landed in 0c8b141

When checking for the validity of the stdio file descriptors (#875), ones which don't exist are intended to be remapped to /dev/null (and, if that doesn't work, we abort). This however doesn't work on all platforms and in all cases, and is not anymore required by POSIX; instead, use the `dup2` syscall as a more robust solution (conforms to POSIX.1). Fixes: nodejs/help#2411 Refs: #875 PR-URL: #44461 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>

nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. needs-ci PRs that need a full CI run. labels Aug 30, 2022

obiwac force-pushed the main branch 6 times, most recently from 69293a1 to b3c392e Compare August 31, 2022 11:02

addaleax reviewed Aug 31, 2022

View reviewed changes

src/node.cc Outdated Show resolved Hide resolved

obiwac force-pushed the main branch from b3c392e to c905070 Compare August 31, 2022 14:15

obiwac requested a review from addaleax August 31, 2022 14:16

addaleax approved these changes Aug 31, 2022

View reviewed changes

theanarkh added the request-ci Add this label to start a Jenkins CI on a PR. label Aug 31, 2022

github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Aug 31, 2022

joyeecheung approved these changes Sep 1, 2022

View reviewed changes

bnoordhuis reviewed Sep 1, 2022

View reviewed changes

src/node.cc Outdated Show resolved Hide resolved

src/node.cc Outdated Show resolved Hide resolved

obiwac force-pushed the main branch 2 times, most recently from da101fe to 326a490 Compare September 1, 2022 11:21

github-actions bot mentioned this pull request Sep 2, 2022

CI Reliability 2022-09-02 nodejs/reliability#365

Open

24 tasks

bnoordhuis approved these changes Sep 2, 2022

View reviewed changes

This was referenced Sep 3, 2022

CI Reliability 2022-09-03 nodejs/reliability#366

Open

CI Reliability 2022-09-04 nodejs/reliability#367

Open

github-actions bot mentioned this pull request Sep 5, 2022

CI Reliability 2022-09-05 nodejs/reliability#368

Open

17 tasks

github-actions bot mentioned this pull request Sep 6, 2022

CI Reliability 2022-09-06 nodejs/reliability#369

Open

17 tasks

bnoordhuis added request-ci Add this label to start a Jenkins CI on a PR. and removed needs-ci PRs that need a full CI run. labels Sep 8, 2022

github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Sep 8, 2022

addaleax added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. request-ci Add this label to start a Jenkins CI on a PR. labels Oct 25, 2022

github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Oct 25, 2022

addaleax added the commit-queue Add this label to land a pull request using GitHub Actions. label Oct 26, 2022

nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Oct 26, 2022

nodejs-github-bot merged commit 0c8b141 into nodejs:main Oct 26, 2022

RafaelGSS mentioned this pull request Nov 1, 2022

v19.1.0 proposal #45269

Merged

danielleadams mentioned this pull request Dec 30, 2022

v18.13.0 release proposal #46025

Merged

JonnyBurger mentioned this pull request Jun 29, 2023

Command failed with EBADF remotion-dev/remotion#2452

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

src: remap invalid file descriptors using `dup2` #44461

src: remap invalid file descriptors using `dup2` #44461

obiwac commented Aug 30, 2022

nodejs-github-bot commented Aug 30, 2022

addaleax commented Aug 31, 2022 •

edited

Loading

obiwac commented Aug 31, 2022

nodejs-github-bot commented Aug 31, 2022

bnoordhuis left a comment

obiwac commented Sep 1, 2022 •

edited

Loading

bnoordhuis commented Sep 2, 2022

obiwac commented Sep 4, 2022

bnoordhuis commented Sep 5, 2022

obiwac commented Sep 6, 2022

bnoordhuis commented Sep 8, 2022

nodejs-github-bot commented Sep 8, 2022

obiwac commented Oct 25, 2022

nodejs-github-bot commented Oct 25, 2022

nodejs-github-bot commented Oct 26, 2022

src: remap invalid file descriptors using dup2 #44461

src: remap invalid file descriptors using dup2 #44461

Conversation

obiwac commented Aug 30, 2022

nodejs-github-bot commented Aug 30, 2022

addaleax commented Aug 31, 2022 • edited Loading

obiwac commented Aug 31, 2022

nodejs-github-bot commented Aug 31, 2022

bnoordhuis left a comment

Choose a reason for hiding this comment

obiwac commented Sep 1, 2022 • edited Loading

bnoordhuis commented Sep 2, 2022

obiwac commented Sep 4, 2022

bnoordhuis commented Sep 5, 2022

obiwac commented Sep 6, 2022

bnoordhuis commented Sep 8, 2022

nodejs-github-bot commented Sep 8, 2022

obiwac commented Oct 25, 2022

nodejs-github-bot commented Oct 25, 2022

nodejs-github-bot commented Oct 26, 2022

src: remap invalid file descriptors using `dup2` #44461

src: remap invalid file descriptors using `dup2` #44461

addaleax commented Aug 31, 2022 •

edited

Loading

obiwac commented Sep 1, 2022 •

edited

Loading